feat: Streamline isolation scope handling & reset in isolation scopes - #22890
feat: Streamline isolation scope handling & reset in isolation scopes#22890mydea wants to merge 2 commits into
Conversation
size-limit report 📦
|
2cab26a to
1eda574
Compare
1eda574 to
ccf3b05
Compare
ccf3b05 to
8979856
Compare
|
|
||
| if (!hasSpansEnabled()) { | ||
| const scopePropagationContext = { ...isolationScope.getPropagationContext(), ...scope.getPropagationContext() }; | ||
| const scopePropagationContext = scope.getPropagationContext(); |
There was a problem hiding this comment.
it seems like this removes a footgun where we potentially incorrectly merged a propagation context before here. I like it!
8979856 to
8196869
Compare
a2496b8 to
7e8cef9
Compare
7e8cef9 to
dea8d51
Compare
dea8d51 to
d1f9cac
Compare
d1f9cac to
2efb184
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 65c8a4c. Configure here.
65c8a4c to
d01ded9
Compare
isaacs
left a comment
There was a problem hiding this comment.
This is a good change, but there are a few gaps, which I think would result in bugs in trace propagation.
I rebased it onto develop, which picked up the fix for one missing case, which was fixed on 46f7431 as part of #22889.
For the others, I added commits with tests and fixes here: https://gist.github.com/isaacs/790e2d23abeebddf5b9079f53e4fbf45 or on the isaacs/fix-fn/streamline-acs-isolation-scope branch.
I figured this is probably more useful than just comments, since I'm going to be out next 2 weeks, so whoever picks this up can inspect and land with those additions, or go a different direction if you decide. 👍
| } = { | ||
| ...isolationScope.getPropagationContext(), | ||
| ...scope.getPropagationContext(), | ||
| }; |
There was a problem hiding this comment.
Should this also be updated to just scope.getPropagationContext()?
| } = scope.getPropagationContext(); |
or:
| } = { | |
| ...scope.getPropagationContext(), | |
| }; |

This PR does two things:
startNewTrace. But this does not align with core/browser handling, so going forward this will be removed. instead, we want this logic to be applied in an isolation scope, so the following code will result in separate traces:without this change, both of these spans would be part of the same trace, which is not what we want in server.
note: In browser, we do not actually fork the isolation scope, there is just a single one, so this does not apply.